HYPERFLEET-549 - feat: standard configuration#71
HYPERFLEET-549 - feat: standard configuration#71rh-amarin wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR restructures configuration into a nested model under clients, adds sentinel metadata, logging and debug sections, and renames many keys from camelCase to snake_case. HyperFleet API settings move to clients.hyperfleet_api, broker topic nests under clients.broker.topic, and environment variables gain a HYPERFLEET_ prefix. LoadConfig now merges CLI flags, env vars, and files with explicit precedence and validation; a new config-dump CLI command prints the merged, redacted YAML. The HyperFleet client constructor now accepts sentinel name and version to set a User-Agent. Helm charts, examples, tests, and docs updated accordingly. Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_Flags as "CLI Flags"
participant Env as "Environment Vars"
participant Config_File as "Config File"
participant Loader as "config.LoadConfig"
participant Logger as "initLogging"
participant App as "Sentinel (serve / config-dump)"
participant Hyperfleet as "HyperFleet Client"
User->>CLI_Flags: provide flags (optional)
User->>Env: set env vars (optional)
User->>Config_File: provide config file (optional)
CLI_Flags->>Loader: flag values
Env->>Loader: env values
Config_File->>Loader: file values
Loader->>Loader: merge (flags > env > file > defaults)
Loader->>Loader: validate & produce redacted copy
Loader->>Logger: pass Log config
Logger->>App: initialize logging
Loader->>Hyperfleet: supply base_url, timeout, sentinel name, version
App->>Hyperfleet: initialize client (User-Agent set)
opt config-dump
User->>App: run config-dump
App->>Loader: load merged config
App->>User: print redacted YAML
end
opt serve
User->>App: run serve
App->>App: start watchers & broker using merged config
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
3 times, most recently
from
96cde3b to
37a82bc
Compare
|
@coderabbitai review |
|
@coderabbitai help |
|
@coderabbitai review |
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
2 times, most recently
from
d30dabd to
a4e1d5f
Compare
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
a4e1d5f to
361a1cd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cmd/sentinel/main.go (1)
379-379:⚠️ Potential issue | 🟠 Major
config-dumpcurrently emits merged config in plaintext.Marshaling
cfgdirectly can leak sensitive values (for example from header maps) into terminal/CI logs. Defaulting to a redacted copy is safer.Suggested safer default
- data, err := yaml.Marshal(cfg) + data, err := yaml.Marshal(cfg.RedactedCopy())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/sentinel/main.go` at line 379, The config-dump currently marshals cfg directly which can leak secrets; instead create and marshal a redacted copy (e.g., call a helper like redactConfig(cfg) or cfg.Redacted()) that copies the config and replaces sensitive fields (authorization/header maps, API keys, tokens, secret strings, TLS private keys, etc.) with placeholder values before calling yaml.Marshal; update the code around the data, err := yaml.Marshal(cfg) line to use the redacted copy so terminal/CI logs never contain raw secrets.
🧹 Nitpick comments (1)
charts/values.yaml (1)
109-113: Unused field may confuse users.
config.clients.broker.topicis defined here but never used incharts/templates/configmap.yaml. The template at line 40 sources the topic from.Values.broker.topic(line 159) instead.Consider either:
- Removing this field entirely, or
- Using this field in the configmap template instead of
.Values.broker.topicThis would prevent users from editing the wrong field when configuring their topic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/values.yaml` around lines 109 - 113, The values.yaml defines config.clients.broker.topic but the configmap template (charts/templates/configmap.yaml) is reading .Values.broker.topic, causing the former to be unused and confusing users; either remove config.clients.broker.topic from values.yaml or update the configmap template to read the topic from .Values.config.clients.broker.topic (replace occurrences of .Values.broker.topic with .Values.config.clients.broker.topic) so the intended field is actually applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configs/dev-example.yaml`:
- Line 26: Update the stale example comment that reads "Logging configuration
(can be overridden by LOG_* env vars or --log-* flags)" to reference the actual
env-var prefix "HYPERFLEET_LOG_*" (and, if relevant, the CLI flag prefix) so the
comment becomes something like "Logging configuration (can be overridden by
HYPERFLEET_LOG_* env vars or --log-* flags)"; locate the string in the file (the
exact comment text) and replace the LOG_* token with HYPERFLEET_LOG_*.
In `@configs/gcp-pubsub-example.yaml`:
- Line 21: Update the logging comment text so the env-var prefix matches the
current config contract: replace the referenced `LOG_*` token in the comment
string with `HYPERFLEET_LOG_*` (the comment that currently reads "Logging
configuration (can be overridden by LOG_* env vars or --log-* flags)") so
operators see the correct env-var prefix; leave the rest of the comment and the
`--log-*` flag text unchanged.
In `@internal/config/config.go`:
- Around line 72-75: BrokerConfig currently only defines Topic but the tests
expect broker.subscription_id; add a SubscriptionID string field to the
BrokerConfig struct (alongside Topic) and annotate it with
yaml:"subscription_id,omitempty" and mapstructure:"subscription_id" so config
file, env and flag loading (tests referencing broker.subscription_id,
HYPERFLEET_BROKER_SUBSCRIPTION_ID / BROKER_SUBSCRIPTION_ID, and CLI flags) can
populate it; update the struct definition in internal/config/config.go
(BrokerConfig) accordingly.
In `@test/integration/test-config-loading.sh`:
- Around line 68-69: The ROOT_DIR calculation is wrong (it only ascends one
level from SCRIPT_DIR); update how ROOT_DIR is derived so it points to the
repository root—for example, change the computation of ROOT_DIR (currently using
SCRIPT_DIR and "..") to ascend two levels (../..) or use a repo-root command
(e.g., git rev-parse --show-toplevel) so references to ROOT_DIR (used to call
$ROOT_DIR/bin/sentinel and run make build) resolve correctly; update the
assignment where ROOT_DIR is defined to use the corrected path resolution.
---
Duplicate comments:
In `@cmd/sentinel/main.go`:
- Line 379: The config-dump currently marshals cfg directly which can leak
secrets; instead create and marshal a redacted copy (e.g., call a helper like
redactConfig(cfg) or cfg.Redacted()) that copies the config and replaces
sensitive fields (authorization/header maps, API keys, tokens, secret strings,
TLS private keys, etc.) with placeholder values before calling yaml.Marshal;
update the code around the data, err := yaml.Marshal(cfg) line to use the
redacted copy so terminal/CI logs never contain raw secrets.
---
Nitpick comments:
In `@charts/values.yaml`:
- Around line 109-113: The values.yaml defines config.clients.broker.topic but
the configmap template (charts/templates/configmap.yaml) is reading
.Values.broker.topic, causing the former to be unused and confusing users;
either remove config.clients.broker.topic from values.yaml or update the
configmap template to read the topic from .Values.config.clients.broker.topic
(replace occurrences of .Values.broker.topic with
.Values.config.clients.broker.topic) so the intended field is actually applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ace1ed10-b248-4ffa-8bff-8c54bcc57c88
📒 Files selected for processing (24)
README.mdcharts/templates/configmap.yamlcharts/templates/deployment.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamldocs/configuration.mddocs/running-sentinel.mdinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_loading_test.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/message-data-blank-id.yamlinternal/config/testdata/minimal.yamlinternal/config/testdata/unknown-field.yamlinternal/config/testdata/valid-complete.yamlinternal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.gotest/integration/test-config-loading.sh
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
2 times, most recently
from
454c33a to
f61b125
Compare
internal/config/config.go
Outdated
| // Validate validates the configuration | ||
| func (c *SentinelConfig) Validate() error { | ||
| if c.Sentinel.Name == "" { | ||
| return fmt.Errorf("sentinel.name is required") |
There was a problem hiding this comment.
Category: JIRA
Per HYPERFLEET-549 acceptance criteria and the configuration standard, validation errors should include remediation guidance showing how to fix the issue via flag, env var, or config file.
Currently all errors are bare messages like "sentinel.name is required". The standard's example shows:
Configuration validation failed:
- Field 'sentinel.name' failed validation: required
Please provide via:
• Flag: --name
• Env: HYPERFLEET_SENTINEL_NAME
• File: sentinel.name
This applies to all return fmt.Errorf(...) calls in Validate() (lines 259-293): sentinel.name, resource_type, clients.hyperfleet_api.base_url, poll_interval, message_decision, and message_data.
|
Category: Inconsistency — The broker topic is now set in two places: as |
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
2 times, most recently
from
79b5fcf to
7ba0b96
Compare
I removed the env variable from the deployment |
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
7ba0b96 to
b50ec30
Compare
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
b50ec30 to
a3828d8
Compare
| TracingEnabled: true, | ||
| Log: LogConfig{ | ||
| Level: "info", | ||
| Format: "json", |
There was a problem hiding this comment.
Category: Inconsistency
NewSentinelConfig() defaults Log.Format to "json" (line 128), but docs/config.md line 64 documents the default as text, and the HyperFleet logging specification also says the default is text.
Either change the code default to "text":
Log: LogConfig{
Level: "info",
Format: "text",
Output: "stdout",
},Or update docs/config.md to say json and document why the sentinel deviates from the logging spec default.
There was a problem hiding this comment.
We will change the default to be JSON, also in the standard
internal/config/config.go
Outdated
| } | ||
|
|
||
| // validationErr returns a validation error for the given field with remediation guidance. | ||
| func validationErr(field, reason string) error { |
There was a problem hiding this comment.
Category: JIRA
The HYPERFLEET-549 acceptance criteria say validation errors must include "actual values", and the configuration standard shows examples like Value 70000 exceeds maximum allowed value of 65535.
validationErr only takes field and reason — there's no way to include the actual value. For "required" fields the value is implicitly empty, but for invalid values (e.g. poll_interval: -1s) showing the actual value helps operators. Consider adding an optional actualValue parameter:
func validationErr(field, reason string, actualValue ...string) error {
var b strings.Builder
fmt.Fprintf(&b, "Field '%s' failed validation: %s\n", field, reason)
if len(actualValue) > 0 && actualValue[0] != "" {
fmt.Fprintf(&b, " Actual value: %s\n", actualValue[0])
}
// ... remediation hints
}There was a problem hiding this comment.
I added this... even though poll_interval seems the only config setting benefiting from it
dev-example.yaml
Outdated
| @@ -0,0 +1,115 @@ | |||
| # Sentinel Configuration Example - Development | |||
There was a problem hiding this comment.
Category: Pattern
This file duplicates configs/dev-example.yaml almost line-for-line. They already diverge: configs/dev-example.yaml sets tracing_enabled: false but this root copy doesn't include it at all.
The project convention keeps config examples under configs/, and the configuration standard references ./configs/config.yaml as the dev default. Consider removing this root-level copy to avoid config drift, or if it serves a different purpose (e.g., quick-start from the repo root), add a comment at the top explaining why it exists separately.
There was a problem hiding this comment.
ouch! good catch, that file was a local copy
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
6 times, most recently
from
f3775f2 to
68c9537
Compare
4 participants
Summary
names both in config files and in config-dump output.
inspect the effective config at runtime or in CI.
hyperfleet-sentinel/<version> (<name>).loads correctly from all three sources (file, env var, CLI flag) and that CLI > env > file precedence is respected.
freshly cross-compiled Linux binary, so the full config path is exercised on Linux in CI.
flat schema.
Test plan
request logs
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests